Skip to content

feat(sync): push git attribution spans, with the hardening that followed - #932

Open
ozymandiashh wants to merge 2 commits into
getagentseal:feat/core-extractionfrom
ozymandiashh:fix/port-sync
Open

feat(sync): push git attribution spans, with the hardening that followed#932
ozymandiashh wants to merge 2 commits into
getagentseal:feat/core-extractionfrom
ozymandiashh:fix/port-sync

Conversation

@ozymandiashh

Copy link
Copy Markdown
Collaborator

Ports three upstream commits this branch never received:

  • 1bf7206 — push git attribution spans with --attribution (the feature)
  • ccee28a — review hardening: cwd-fallback egress, Windows paths, PR-link validation
  • 50c8251close credential-leak paths, session retraction, span/key/CLI hardening

grep -ri attribution packages/cli/src/sync/ returned nothing on this branch; upstream's push.ts has several hits. The whole feature and its security follow-up were missing.

Ported as an end state, not in sequence

Two and three revise one. Replaying them in order would have introduced the issues they fix and then removed them again — and anything missed on the third pass would have shipped a feature with a reopened security hole in the same change. That is the specific way this port could have gone wrong, so the credential-leak paths from 50c8251 are enumerated and checked off individually against the result rather than assumed to arrive with the feature.

Scope

packages/cli/src/sync/{cli,otlp,push}.ts, yield.ts, the mock IdP fixture, docs, changelog, plus two new test files covering the feature and its CLI surface.

Nothing here touches the id derivations in otlp.ts — those are #931's subject. No new unkeyed digest is introduced.

Verification

34 new attribution tests green, 93 across the seven sibling yield/sync suites, core 509, typecheck clean in both packages. The Playwright sync E2E self-skips without its three env vars, so it stayed inert.

@ozymandiashh
ozymandiashh marked this pull request as draft August 5, 2026 10:03
@ozymandiashh

Copy link
Copy Markdown
Collaborator Author

Converted to draft: this was opened with only a self-review by the model that wrote it, which is not the review bar this batch is held to. Independent review by two other reviewers is running now; I will mark it ready once both have passed and any findings are applied. Apologies for the noise.

@ozymandiashh

Copy link
Copy Markdown
Collaborator Author

Merge-order blocker, from independent re-review.

This PR adds a new attribution payload that sends device_id, span ids and trace ids — all still derived with bare SHA-256 on this branch — and introduces stateHash over user-derived state. #931 is the PR that keys those derivations, and it is not an ancestor of this branch.

So landing this alone ships new consumers of the unkeyed construction, and my earlier claim in the commit message that this is "not a D1 violation" is contested: one reviewer argued stateHash hides nothing because its inputs ride in cleartext in the same payload, another argued the derivations it feeds are exactly what D1 governs. The second reading is the safer one to act on.

Concrete requirement: #931 must land before this, and whoever merges should reconcile the one-argument call sites this PR adds against #931's keyed signatures. I would rather flag this than have it discovered at merge.

@ozymandiashh
ozymandiashh marked this pull request as ready for review August 5, 2026 13:39
@ozymandiashh

Copy link
Copy Markdown
Collaborator Author

Correction to the note above, from the final cross-PR review.

I wrote "land #931 first, or reconcile the attribution call sites into it." The or is wrong: git merge-tree reports no conflict in either direction, and the merged file does not compile regardless of order — #931 removes the createHash import that this PR's stateHash uses, and this PR's two call sites use the one-argument signatures #931 replaces. Rebasing replays the same call sites.

Reconciliation is mandatory, not an alternative. A reconciliation commit threading privacyKey into the attribution derivations has to land with this PR.

Worth stating why it matters beyond the build: if someone resolves it by loosening the signature instead, those two call sites are exactly the unkeyed span ids #931 exists to eliminate — reintroduced in brand-new code.

The sync path derived three identifiers with bare SHA-256 and sent them to a
configured endpoint.

`deriveDeviceId` hashed `hostname:username` and truncated to 64 bits, commented
"pseudonymous, stable". An unkeyed digest of a host and username pair is not
pseudonymous against anyone who can guess plausible values: hash the guess,
compare, done. `deriveSpanId` hashed the dedup key — and for pi, zerostack,
lingtai-tui and codebuff that key embeds the raw absolute source path, home
directory included, because the bridge passes `source.path` straight through.
Guess a plausible home and project name and the same confirmation works.

This is the project's own standard, not an outside opinion. Decision D1 requires
a caller-supplied HMAC key for fingerprints precisely so digests of paths cannot
be dictionary-attacked, and core's fingerprint module throws on an empty key to
enforce it. The sync path bypassed the primitive entirely. It also contradicted
the project's own user-facing guarantee: docs/sync/README.md promises that
code, file contents, diffs and PATHS stay local, and the unkeyed span id shipped
absolute paths (for the four providers above) in a form confirmable by anyone
with a plausible guess.

All three ids are now HMAC-SHA256 under the per-install privacy key — the same
key core's fingerprints use — with domain prefixes so one value in two
positions never yields the same digest, and composite inputs joined with the
same ASCII Unit Separator (0x1f) core/fingerprint.ts uses so a value containing
':' cannot forge a field boundary. The derive functions throw on an empty key
rather than degrading. The payload builder obtains the key itself, so the
decode path, which runs with an empty key by design, never reaches it.

Sync now REQUIRES the persisted key: privacy-key.ts exposes a strict variant
that aborts the push instead of falling back to per-process randomness when the
config dir is unwritable, and refuses to silently regenerate a key file that
fails validation (truncated by a full disk, a partial write). Cross-process id
stability is load-bearing — partially rejected batches are not ledgered
precisely because deterministic span ids make full-batch retry safe — so a
per-process fallback key would emit fresh ids on every retry and let the backend
double-count accepted spans, and a silent re-key would orphan everything already
pushed. The fingerprint consumers keep the tolerant fallback: they only need
per-process stability.

The refusal is now complete, and enforced for every corrupt shape: "no file at
all" is the only state a first use may create. A file that exists but is
unreadable, zero-byte or whitespace-only (a partial write), or fails hex
validation aborts the push and is left untouched — treating those as MISSING
would silently regenerate the file and re-key every id, which is exactly the
case the strict path exists to refuse. First creation is also exclusive
(O_CREAT|O_EXCL): when two processes race the first use, the loser re-reads and
adopts the winner's key, so concurrent pushes can never mint different keys and
mix cached device ids with spans derived from the other.

Scope, stated honestly: sync is opt-in and needs an endpoint plus credentials,
the digests are of identifiers rather than prompts or file contents, and this
predates the extraction. It is not an active leak of user content. It is a weak
construction the project already knows how to do properly. This change narrows
the exposure rather than closing it: ai.project still ships a project name in
the clear, and in one Claude fallback path that name is a dash-encoded absolute
path.

Blast radius: every id is re-keyed once at upgrade, so anything already pushed
stops correlating with new sends and the backend sees a fresh device identity.
Ids stay stable afterwards unless the key file is lost. The host-side sent
ledger keys off the raw dedup key and is unaffected, so re-push filtering keeps
working.
Ports three upstream commits this branch never received: the attribution
feature (1bf7206), the review hardening on top of it (ccee28a), and the
security follow-up that closed credential-leak paths and added session
retraction (50c8251).

They are ported as an end state rather than in sequence. Two and three revise
one, so replaying them in order would have introduced the very issues they fix
and then removed them again — and anything missed in the third pass would have
shipped a feature with a reopened hole, which is the specific way this port
could have gone wrong. The credential-leak paths that commit closes are
enumerated and checked off individually against the result.

One correction to an earlier draft of this message, which claimed no new
unkeyed digest is introduced. That was wrong: stateHash in sync/otlp.ts is a
new unkeyed sha256, and it feeds deriveSpanId, so it is an input to a value
that goes on the wire. It is not a D1 violation — D1 governs core's fingerprint
module and its caller-supplied key, while stateHash is a local ledger
discriminator computed over data that is itself sent in cleartext, so it hides
nothing and leaks nothing. But the sentence was false and is worth correcting
rather than quietly dropping.

This branch now carries getagentseal#931's commit (c467548, "fix(sync): key the device,
span and trace digests") beneath this one — cherry-picked onto the shared base
so the history stays two clean commits. That ordering is load-bearing:
reconciliation is mandatory in every merge order, not optional. git merge-tree
reports no conflict against getagentseal#931 in either direction, yet the merged file does
not compile: getagentseal#931 drops the createHash import and gives the derive functions a
privacyKey first argument, so an unreconciled attribution section leaves
stateHash with an undefined symbol and two one-argument call sites. Rebasing
replays the same breakage, which is why the earlier "land getagentseal#931 first, OR
reconcile" framing was wrong.

The two call sites are reconciled INTO getagentseal#931's keyed signatures, in the
direction getagentseal#931 demands: buildAttributionOtlpPayload obtains the persisted host
privacy key exactly as buildOtlpPayload does — one getPersistedHostPrivacyKey
call per builder, no second source of the key — and threads it into
deriveTraceId and deriveSpanId. This is the security point of the
reconciliation: loosening the signatures back to one argument would
reintroduce exactly the unkeyed span and trace ids getagentseal#931 exists to remove, in
new code.

stateHash stays unkeyed, deliberately: it is a local ledger discriminator over
One more merge-compat fix, in getagentseal#931's own test file (sync-privacy-key.test.ts):
the concurrency fixture path was built from process.cwd(), which is the repo
root under `--root packages/cli` — the worker then exited on a nonexistent
file before writing its ready file and the race test timed out. The path is
now anchored to the test file's own location (fileURLToPath(import.meta.url)).
This is the only line of getagentseal#931's tree this branch touches; 37a5b46 remains a
verbatim copy of c467548.

Second fix in getagentseal#931's tree, same motivation: the concurrency race test
adopted with only a 50ms budget. createKeyFileExclusive polled the winner's
file 5x10ms after EEXIST, and the strict entry check refused an 'invalid'
file INSTANTLY — but the winner's create (open) and write are separate
syscalls, and under load the loser can read the still-empty file either at
entry or inside the poll. Both windows now share one bounded awaitValidKey
(500ms) that ADOPTS the winner's key when it lands and otherwise throws the
same refusal. Nothing is ever overwritten; a file left invalid by a crash or
truncated write still fails loudly. This is the second getagentseal#931 file this branch
touches; 37a5b46 remains a verbatim copy of c467548.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant